Skip to content

feat(core, react): add step up mfa error handler#88

Closed
NaveenChand755 wants to merge 36 commits into
mainfrom
feat/step-up-handler
Closed

feat(core, react): add step up mfa error handler#88
NaveenChand755 wants to merge 36 commits into
mainfrom
feat/step-up-handler

Conversation

@NaveenChand755
Copy link
Copy Markdown
Contributor

@NaveenChand755 NaveenChand755 commented Feb 18, 2026

Changes

This PR introduces MFA step-up authentication support across both SPA and proxy modes, replaces the withServices + ScopeManagerProvider pattern with a centralized GateKeeper component, and refactors all block components into a container → guard → view architecture.

Motivation

  1. Scope handling was tightly coupled to rendering
    Block components relied on withServices + ScopeManagerProvider for authorization, which:
  • Coupled scope acquisition to rendering
  • Increased complexity
  • Made testing difficult
  • Spread authorization logic across multiple layers
  1. No built-in MFA step-up handling
    There was no transparent handling for HTTP 403 mfa_required errors.

  2. Hook indirection added unnecessary complexity
    The hooks were split into paired files (e.g., use-domain-table and use-domain-table-logic), but the separation of concerns between them is not very clear.

Core Package (@auth0/universal-components-core)

Added

Step-Up API Service

  1. New services/step-up module
  2. initializeStepUpApiService() supports:
  • SPA mode (Auth0 SDK MFA client)
  • Proxy mode (fetch-based MFA client for enroll, challenge, verify, getAuthenticators)

Step-Up Utilities

  • isMfaRequiredError() type guard for detecting mfa_required errors across multiple error shapes

SPA Token Retriever

  • createSpaTokenRetriever() replaces TokenManager
  • Handles consent_required and login_required via popup fallback
  • Simplified token architecture

Removed

  • TokenManager (replaced by SpaTokenRetriever)

Updated

  • myAccountApiClient
  • myOrganizationApiClient
  • Adapted to the new auth/token architecture

React Package (@auth0/universal-components-react)

New: GateKeeper
Central guard component that intercepts loading, error, and MFA states before rendering children.

Behavior:

  • Loading → renders Spinner
  • 403 mfa_required → triggers full MFA step-up dialog
  • 500+ server errors → renders ErrorFallback with retry
  • All other errors → passed through to children

Architecture Refactor (All 6 Block Components)

New pattern:

Container → GateKeeper → View

Container

  • Calls the hook
  • Passes loading/error into GateKeeper
  • Passes data to View

GateKeeper

  • Guards rendering during loading, MFA, and server error states

View

  • Purely presentational
  • No loading or error logic

Refactored components:

  • OrganizationDetailsEdit
  • DomainTable
  • SsoProviderCreate
  • SsoProviderEdit
  • SsoProviderTable
  • UserMfaManagement

Removed

  • withServices HOC
  • ScopeManagerProvider
  • useScopeManager
  • use-domain-table-logic
  • use-sso-provider-create-logic
  • use-sso-provider-edit-logic
  • use-sso-provider-table-logic

Logic consolidated into primary hooks.

Updated

useErrorHandler

  • Skips MFA and 500+ errors (handled by GateKeeper)
  • Shows toast only for client-recoverable errors

SPA/Proxy providers

  • Removed scope manager integration

Types

  • Explicit view prop interfaces

Key Improvements

  • Centralized MFA step-up handling
  • Clear separation of concerns
  • Improved testability
  • Reduced abstraction layers
  • Simpler token architecture
  • Cleaner container/view composition
  • More maintainable TypeScript types

References

Screenshot 2026-02-18 at 11 09 15 PM

Testing

Screenshot 2026-03-04 at 11 34 47 AM Screenshot 2026-03-04 at 11 34 57 AM Screenshot 2026-03-04 at 12 18 15 AM Screenshot 2026-03-04 at 11 34 41 AM
  • This change adds unit test coverage
  • This change has been tested on the latest version of the platform/language or why not

Checklist

@NaveenChand755 NaveenChand755 added enhancement New feature or request Gen AI Indicates that the most of the code in this PR were generated or assisted by generative AI tools. labels Feb 18, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 18, 2026

🚀 Preview deployment

Branch: refs/pull/88/merge
Commit: 426e1a0

📝 Preview URL: https://auth0-universal-components-ptysuvg7w-okta.vercel.app


Updated at 2026-03-10T13:03:02.482Z

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 2, 2026

Codecov Report

❌ Patch coverage is 76.58206% with 692 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.29%. Comparing base (6e495cd) to head (f96aa37).
⚠️ Report is 310 commits behind head on main.

Files with missing lines Patch % Lines
...ed/mfa-step-up/step-up-qr-code-enrollment-form.tsx 11.35% 164 Missing ⚠️
.../shared/mfa-step-up/step-up-contact-input-form.tsx 11.76% 150 Missing ⚠️
...ared/mfa-step-up/step-up-enrollment-setup-form.tsx 43.82% 132 Missing ⚠️
...uth0/shared/mfa-step-up/step-up-challenge-form.tsx 8.26% 111 Missing ⚠️
...es/react/src/hooks/shared/use-step-up-challenge.ts 43.80% 59 Missing ⚠️
...rc/hooks/my-organization/use-sso-provider-table.ts 82.25% 33 Missing ⚠️
...s/react/src/components/auth0/shared/gatekeeper.tsx 92.60% 17 Missing ⚠️
.../shared/mfa-step-up/step-up-authenticator-list.tsx 84.70% 13 Missing ⚠️
...c/hooks/my-organization/use-sso-provider-create.ts 89.41% 9 Missing ⚠️
...ct/src/hooks/my-organization/use-sso-domain-tab.ts 97.52% 3 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
- Coverage   88.20%   84.29%   -3.91%     
==========================================
  Files         145      151       +6     
  Lines       12533    13331     +798     
  Branches     1603     1375     -228     
==========================================
+ Hits        11055    11238     +183     
- Misses       1478     2093     +615     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@NaveenChand755 NaveenChand755 marked this pull request as ready for review March 4, 2026 04:42
Comment on lines -108 to -117
if (isFetchLoading) {
return (
<div
style={currentStyles.variables}
className="flex items-center justify-center min-h-96 w-full"
>
<Spinner />
</div>
);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this is missing or intentionally removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handled by gatekeeper now

Comment on lines -173 to -180
if (isLoading || isLoadingConfig || isLoadingIdpConfig) {
return (
<div className="flex justify-center items-center p-8">
<Spinner />
</div>
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar thing here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Comment on lines +164 to +203
const LoadingState = () => (
<div className="flex items-center justify-center p-8">
<Spinner />
</div>
);

const ErrorState = () => (
<div className="text-center text-destructive py-4">{t('error.mfa.fetch_failed')}</div>
);

const EmptyState = () => (
<div className="text-center text-muted-foreground py-4">{t('error.mfa.no_authenticators')}</div>
);

const AuthenticatorList = ({ items }: { items: StepUpAuthenticator[] }) => (
<div className="space-y-2 py-4">
{items.map((auth) => (
<div key={auth.id} className="border rounded p-3">
<div className="font-medium">{auth.name || auth.authenticatorType}</div>
<div className="text-sm text-muted-foreground">
Type: {auth.authenticatorType} | Active: {auth.active ? 'Yes' : 'No'}
</div>
</div>
))}
</div>
);

const EnrollmentList = ({ factors }: { factors: EnrollmentFactor[] }) => (
<div className="space-y-2 py-4">
<div className="text-sm text-muted-foreground text-center mb-4">
{t('error.mfa.enrollment_required')}
</div>
{factors.map((factor) => (
<div key={factor.type} className="border rounded p-3">
<div className="font-medium">{factor.type}</div>
<div className="text-sm text-muted-foreground">{t('error.mfa.factor_available')}</div>
</div>
))}
</div>
);
Copy link
Copy Markdown
Contributor

@rax7389 rax7389 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move them outside of gatekeeper and use it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes , was in a pr opened by harish , its merged into this branch now

setIsRetrying(true);
try {
await onRetry();
setIsMfaDialogOpen(true);
Copy link
Copy Markdown
Contributor

@rax7389 rax7389 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we need to reopen mfa dialog on every retry? should there be any conditional logic here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored it

Comment on lines +23 to +32
const ACTION_CANCELLED_ERROR = 'ACTION_CANCELLED';

const isActionCancelledError = (error: unknown): boolean => {
return error instanceof Error && error.message === ACTION_CANCELLED_ERROR;
};

export interface UseSsoProvisioningOptions {
provisioning?: SsoProvisioningTabEditProps;
customMessages?: Record<string, unknown>;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be reusable from utils I saw the same in scim token hook

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup moved it into lib/utils

const domain = auth.domain ?? auth.contextInterface.getConfiguration()?.domain;
if (!domain) throw new Error('SpaTokenRetriever: Auth0 domain is not configured');

const audience = buildAudience(domain, audiencePath);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should define what will happen with empty audience path, need to add sanity check here

Copy link
Copy Markdown
Contributor Author

@NaveenChand755 NaveenChand755 Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding a guard here isn't necessary since:

This is an internal function — no external consumers call getToken directly
All callers hardcode the audience path ('me', 'my-org')

Comment on lines +39 to +47
return {
/**
* Retrieves an access token for the specified scope and audience.
* @param scope - The OAuth scope to request.
* @param audiencePath - The API audience path segment.
* @param ignoreCache - Whether to bypass the token cache.
* @returns The access token, or undefined if using proxy mode.
*/
async getToken(
Copy link
Copy Markdown
Contributor

@rax7389 rax7389 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will there be any scope of adding further method under this? if not we should consider returning the anonymous function instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as of now , no , but we might need to extend it in future ! will leave it as is

* @returns Proxy-based MFA client.
*/
function createProxyMfaClient(authProxyUrl: string): Omit<MfaApiClient, 'getEnrollmentFactors'> {
const baseUrl = authProxyUrl.replace(/\/$/, '');
Copy link
Copy Markdown
Contributor

@rax7389 rax7389 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before we do any operations here we should wrap authProxyUrl with new URL(authProxyUrl) for sanity

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authProxyUrl is a relative path (e.g., / or /api/auth), so new URL() would throw since it requires an absolute URL — the simple trailing-slash strip is sufficient here and the value is developer-configured, not user input.


return {
getAuthenticators: async (mfaToken: string) => {
const response = await fetch(`${baseUrl}/auth/mfa/authenticators?mfa_token=${mfaToken}`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we wrap mfaToken with encodeURIComponent?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed !, updated

Comment on lines +79 to +106
const response = await fetch(`${baseUrl}/auth/mfa/enroll`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(body),
});
return handleResponse<EnrollmentResponse>(response);
},

challenge: async (params: ChallengeAuthenticatorParams) => {
const response = await fetch(`${baseUrl}/auth/mfa/challenge`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
mfaToken: params.mfaToken,
challengeType: params.challengeType,
authenticatorId: params.authenticatorId,
}),
});
return handleResponse<ChallengeResponse>(response);
},

verify: async (params: VerifyParams) => {
const response = await fetch(`${baseUrl}/auth/mfa/verify`, {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify(params),
});
return handleResponse<TokenEndpointResponse>(response);
Copy link
Copy Markdown
Contributor

@rax7389 rax7389 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bit of common pattern here for every mfa action, can we consider reusing with a common request function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated , created an new utils in api folder

audiencePath: string,
ignoreCache = false,
): Promise<string | undefined> {
if (auth.authProxyUrl) return undefined;
Copy link
Copy Markdown
Contributor

@rax7389 rax7389 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check can be placed on the callee function to avoid invocation of getToken itself in the first place

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed , since this is never used in proxy mode , will move the check to the callee

Comment on lines +75 to +77
const auth: AuthDetails = {
authProxyUrl: 'https://proxy.example.com/',
};
Copy link
Copy Markdown
Contributor

@rax7389 rax7389 Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this auth object is created repeatedly in multiple it statements, we should reuse if there is no reference changes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed , updated

className="text-sm"
variant="outline"
size="default"
variant="ghost"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change will impact existing mfa as well

<CardTitle className="text-base font-semibold">{displayName}</CardTitle>
{formattedDate && (
<CardDescription className="text-xs">
{t('error.mfa.registered_on').replace('${date}', formattedDate)}
Copy link
Copy Markdown
Contributor

@rax7389 rax7389 Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why do we need to use replace here
t('error.mfa.registered_on', { date: formattedDate })
this way should work

name="userOtp"
render={({ field }) => (
<FormItem>
<FormLabel className="text-sm font-medium" htmlFor="step-up-otp-input">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a potential accessibility bug here the textfield depends on isRecoveryCode but our form label will always look for id with step-up-otp-input

variant="primary"
size="sm"
disabled={
!userOtp?.trim() || (!isRecoveryCode && userOtp.length !== 6) || isVerifying
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good in terms of readability if we can have

const isSubmitDisabled =
  isVerifying || !userOtp?.trim() || (!isRecoveryCode && userOtp.length !== 6);

then
disabled={isSubmitDisabled}

Comment on lines +76 to +80
const typeToTranslationKey: Record<string, string> = {
'push-notification': 'push',
phone: 'sms',
totp: 'otp',
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this const is repeated saw in step-up-authenticator-list as well

Comment on lines +43 to +51
const map: Record<string, MFAType> = {
otp: FACTOR_TYPE_TOTP,
totp: FACTOR_TYPE_TOTP,
'push-notification': FACTOR_TYPE_PUSH_NOTIFICATION,
sms: FACTOR_TYPE_PHONE,
phone: FACTOR_TYPE_PHONE,
email: FACTOR_TYPE_EMAIL,
'recovery-code': FACTOR_TYPE_RECOVERY_CODE,
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this constant map is getting recreated inside a loop we should segregate it from this function

const renderInstallationPhase = () => (
<div className="w-full max-w-sm mx-auto">
<div className="flex flex-col items-center justify-center flex-1 space-y-10">
<p className={cn('text-center text-primary text-sm font-normal')}>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need cn for static classes

if (!otpData?.barcodeUri) {
fetchOtpEnrollment();
}
}, [otpData?.barcodeUri]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are missing fetchOtpEnrollment dep here

<div style={currentStyles.variables} className="w-full max-w-sm mx-auto text-center">
<div className="space-y-6">
<div>
<p className={cn('font-normal block text-sm text-center mb-4 text-primary')}>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar as above

expires_in: 3600,
}),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should separate everything related to MFA into different files (types, mocks, etc).
This is a new API that should not be related to coreClient

getEnrollmentFactors(mfaToken: string): Promise<EnrollmentFactor[]>;
verify(params: VerifyParams): Promise<TokenEndpointResponse>;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, separate all types for MFA in a different file

},
stepUpApiService: undefined,
getStepUpApiService: function () {
return undefined as unknown as ReturnType<CoreClientInterface['getStepUpApiService']>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should never use casts unless there is no other option.
Here this function can just return ReturnType<CoreClientInterface['getStepUpApiService']> | undefined or probably follow the same approach you have in getMyAccountApiClient, using throw new Error('Function not implemented.');

return undefined;
},
stepUpApiService: undefined,
getStepUpApiService: function () {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why getStepUpApiService instead of getStepUpApiClient` to follow the naming convention we already have in this file?

@@ -1,7 +1,46 @@
{
"common": {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of these messages are not "common", they are part of a new component!

return client;
};

return client;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, check ALL services and do not cast them

*/
export function initializeStepUpApiService(auth: AuthDetails): StepUpApiService {
if (auth.authProxyUrl) {
return createProxyMfaClient(auth.authProxyUrl) as StepUpApiService;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return correct types instead casting this

@@ -893,7 +893,7 @@ interface ComponentAction<T, U = undefined> {
<li>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have designs for this? I think the UI experience is not aligned with the rest of the components.

Image
  • Texts must be aligned to the left, not centered
  • Buttons must be on the bottom and right side
  • Cancel button must be outlined

Some examples in other components:

Image Image

@@ -893,7 +893,7 @@ interface ComponentAction<T, U = undefined> {
<li>
<code>tabs.provisioning.content.notifications.*</code> – Notification messages
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing between input and error is too big:

Image

@@ -893,7 +893,7 @@ interface ComponentAction<T, U = undefined> {
<li>
<code>tabs.provisioning.content.notifications.*</code> – Notification messages
(delete_success, remove_success, update_success, general_error,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Designs are not aligned:

  • Not sure about the cards, but probably we should use separators instead
  • Should Verify button be primary?

Modal:
Image

MFA component:
Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Gen AI Indicates that the most of the code in this PR were generated or assisted by generative AI tools.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants